Prepare datamodel for multi-environment#1765
Open
juliusmarminge wants to merge 17 commits intot3code/pr-1708/web/atomic-store-refactorfrom
Open
Prepare datamodel for multi-environment#1765juliusmarminge wants to merge 17 commits intot3code/pr-1708/web/atomic-store-refactorfrom
juliusmarminge wants to merge 17 commits intot3code/pr-1708/web/atomic-store-refactorfrom
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
size:XXL
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Redundant no-op identity map in effect pipeline
- Removed the no-op
Effect.map((option) => option)line that was a leftover from refactoring.
- Removed the no-op
- ✅ Fixed: Replay events enriched twice in subscription handler
- Moved
enrichProjectEventto only wrap the livestreamDomainEventsstream so already-enriched replay events skip the redundant enrichment pass.
- Moved
- ✅ Fixed: Selector creates unstable array references every call
- Added a per-projectScopedId reference-equality cache that returns the previous result array when the underlying scopedIds and sidebarMap references haven't changed.
Or push these changes by commenting:
@cursor push 481ff4254f
Preview (481ff4254f)
diff --git a/apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts b/apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts
--- a/apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts
+++ b/apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts
@@ -748,7 +748,6 @@
"ProjectionSnapshotQuery.getActiveProjectByWorkspaceRoot:decodeRow",
),
),
- Effect.map((option) => option),
Effect.flatMap((option) =>
Option.isNone(option)
? Effect.succeed(Option.none<OrchestrationProject>())
diff --git a/apps/server/src/ws.ts b/apps/server/src/ws.ts
--- a/apps/server/src/ws.ts
+++ b/apps/server/src/ws.ts
@@ -503,7 +503,10 @@
Effect.catch(() => Effect.succeed([] as Array<OrchestrationEvent>)),
);
const replayStream = Stream.fromIterable(replayEvents);
- const source = Stream.merge(replayStream, orchestrationEngine.streamDomainEvents);
+ const liveStream = orchestrationEngine.streamDomainEvents.pipe(
+ Stream.mapEffect(enrichProjectEvent),
+ );
+ const source = Stream.merge(replayStream, liveStream);
type SequenceState = {
readonly nextSequence: number;
readonly pendingBySequence: Map<number, OrchestrationEvent>;
@@ -515,43 +518,33 @@
return source.pipe(
Stream.mapEffect((event) =>
- enrichProjectEvent(event).pipe(
- Effect.flatMap((enrichedEvent) =>
- Ref.modify(
- state,
- ({
- nextSequence,
- pendingBySequence,
- }): [Array<OrchestrationEvent>, SequenceState] => {
- if (
- enrichedEvent.sequence < nextSequence ||
- pendingBySequence.has(enrichedEvent.sequence)
- ) {
- return [[], { nextSequence, pendingBySequence }];
- }
+ Ref.modify(
+ state,
+ ({
+ nextSequence,
+ pendingBySequence,
+ }): [Array<OrchestrationEvent>, SequenceState] => {
+ if (event.sequence < nextSequence || pendingBySequence.has(event.sequence)) {
+ return [[], { nextSequence, pendingBySequence }];
+ }
- const updatedPending = new Map(pendingBySequence);
- updatedPending.set(enrichedEvent.sequence, enrichedEvent);
+ const updatedPending = new Map(pendingBySequence);
+ updatedPending.set(event.sequence, event);
- const emit: Array<OrchestrationEvent> = [];
- let expected = nextSequence;
- for (;;) {
- const expectedEvent = updatedPending.get(expected);
- if (!expectedEvent) {
- break;
- }
- emit.push(expectedEvent);
- updatedPending.delete(expected);
- expected += 1;
- }
+ const emit: Array<OrchestrationEvent> = [];
+ let expected = nextSequence;
+ for (;;) {
+ const expectedEvent = updatedPending.get(expected);
+ if (!expectedEvent) {
+ break;
+ }
+ emit.push(expectedEvent);
+ updatedPending.delete(expected);
+ expected += 1;
+ }
- return [
- emit,
- { nextSequence: expected, pendingBySequence: updatedPending },
- ];
- },
- ),
- ),
+ return [emit, { nextSequence: expected, pendingBySequence: updatedPending }];
+ },
),
),
Stream.flatMap((events) => Stream.fromIterable(events)),
diff --git a/apps/web/src/store.ts b/apps/web/src/store.ts
--- a/apps/web/src/store.ts
+++ b/apps/web/src/store.ts
@@ -1305,22 +1305,36 @@
]
: undefined;
+const _threadIdsByProjectCache = new Map<
+ string,
+ { scopedIds: string[]; sidebarMap: Record<string, SidebarThreadSummary>; result: ThreadId[] }
+>();
+
export const selectThreadIdsByProjectId =
(projectId: ProjectId | null | undefined) =>
- (state: AppState): ThreadId[] =>
- projectId
- ? (
- state.threadScopedIdsByProjectScopedId[
- getProjectScopedId({
- environmentId: state.activeEnvironmentId,
- id: projectId,
- })
- ] ?? EMPTY_SCOPED_IDS
- )
- .map((scopedId) => state.sidebarThreadsByScopedId[scopedId]?.id ?? null)
- .filter((threadId): threadId is ThreadId => threadId !== null)
- : EMPTY_THREAD_IDS;
+ (state: AppState): ThreadId[] => {
+ if (!projectId) return EMPTY_THREAD_IDS;
+ const projectScopedId = getProjectScopedId({
+ environmentId: state.activeEnvironmentId,
+ id: projectId,
+ });
+ const scopedIds = state.threadScopedIdsByProjectScopedId[projectScopedId] ?? EMPTY_SCOPED_IDS;
+ const sidebarMap = state.sidebarThreadsByScopedId;
+
+ const cached = _threadIdsByProjectCache.get(projectScopedId);
+ if (cached && cached.scopedIds === scopedIds && cached.sidebarMap === sidebarMap) {
+ return cached.result;
+ }
+
+ const result = scopedIds
+ .map((scopedId) => sidebarMap[scopedId]?.id ?? null)
+ .filter((threadId): threadId is ThreadId => threadId !== null);
+
+ _threadIdsByProjectCache.set(projectScopedId, { scopedIds, sidebarMap, result });
+ return result;
+ };
+
export function setError(state: AppState, threadId: ThreadId, error: string | null): AppState {
return updateThreadState(state, state.activeEnvironmentId, threadId, (t) => {
if (t.error === error) return t;You can send follow-ups to the cloud agent here.
apps/server/src/orchestration/Layers/ProjectionSnapshotQuery.ts
Outdated
Show resolved
Hide resolved
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Contributor
ApprovabilityVerdict: Needs human review 8 blocking correctness issues found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Unreachable fallback values in
resolveServerUrl- Removed the redundant third and fourth arguments from firstNonEmptyString since resolvePrimaryEnvironmentBootstrapUrl() already contains the complete fallback chain and always returns a non-empty string.
- ✅ Fixed: Cache returns stale null for later-configured remotes
- Skip caching null identity results so directories without a git remote are re-resolved on subsequent calls, allowing later-configured remotes to be detected.
Or push these changes by commenting:
@cursor push ffbf2a1e3c
Preview (ffbf2a1e3c)
diff --git a/apps/server/src/project/Layers/RepositoryIdentityResolver.ts b/apps/server/src/project/Layers/RepositoryIdentityResolver.ts
--- a/apps/server/src/project/Layers/RepositoryIdentityResolver.ts
+++ b/apps/server/src/project/Layers/RepositoryIdentityResolver.ts
@@ -115,12 +115,14 @@
}
const resolved = yield* Effect.promise(() => resolveRepositoryIdentity(cwd));
- yield* Ref.update(cacheRef, (current) => {
- const next = new Map(current);
- next.set(cwd, resolved.identity);
- next.set(resolved.cacheKey, resolved.identity);
- return next;
- });
+ if (resolved.identity !== null) {
+ yield* Ref.update(cacheRef, (current) => {
+ const next = new Map(current);
+ next.set(cwd, resolved.identity);
+ next.set(resolved.cacheKey, resolved.identity);
+ return next;
+ });
+ }
return resolved.identity;
});
diff --git a/apps/web/src/lib/utils.ts b/apps/web/src/lib/utils.ts
--- a/apps/web/src/lib/utils.ts
+++ b/apps/web/src/lib/utils.ts
@@ -53,12 +53,7 @@
pathname?: string | undefined;
searchParams?: Record<string, string> | undefined;
}): string => {
- const rawUrl = firstNonEmptyString(
- options?.url,
- resolvePrimaryEnvironmentBootstrapUrl(),
- import.meta.env.VITE_WS_URL,
- window.location.origin,
- );
+ const rawUrl = firstNonEmptyString(options?.url, resolvePrimaryEnvironmentBootstrapUrl());
const parsedUrl = new URL(rawUrl);
if (options?.protocol) {You can send follow-ups to the cloud agent here.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Nullish coalescing collapses null into fallback incorrectly
- Replaced
environmentId ?? resolveCurrentEnvironmentId()withenvironmentId !== undefined ? environmentId : resolveCurrentEnvironmentId()so explicitnullis preserved.
- Replaced
- ✅ Fixed: Multiple exported functions are unused outside definitions
- Removed
scopeThreadSessionRef,resolveEnvironmentClient,tagEnvironmentValue,attachEnvironmentDescriptor, and their supporting types (EnvironmentClientRegistry,EnvironmentScopedRef) from client-runtime.
- Removed
Or push these changes by commenting:
@cursor push 4730c6f1c2
Preview (4730c6f1c2)
diff --git a/apps/web/src/routes/__root.tsx b/apps/web/src/routes/__root.tsx
--- a/apps/web/src/routes/__root.tsx
+++ b/apps/web/src/routes/__root.tsx
@@ -531,7 +531,10 @@
try {
const snapshot = await api.orchestration.getSnapshot();
if (!disposed) {
- syncServerReadModel(snapshot, environmentId ?? resolveCurrentEnvironmentId());
+ syncServerReadModel(
+ snapshot,
+ environmentId !== undefined ? environmentId : resolveCurrentEnvironmentId(),
+ );
reconcileSnapshotDerivedState();
if (recovery.completeSnapshotRecovery(snapshot.snapshotSequence)) {
void runReplayRecovery("sequence-gap");
diff --git a/packages/client-runtime/src/knownEnvironment.ts b/packages/client-runtime/src/knownEnvironment.ts
--- a/packages/client-runtime/src/knownEnvironment.ts
+++ b/packages/client-runtime/src/knownEnvironment.ts
@@ -1,4 +1,4 @@
-import type { EnvironmentId, ExecutionEnvironmentDescriptor } from "@t3tools/contracts";
+import type { EnvironmentId } from "@t3tools/contracts";
export interface KnownEnvironmentConnectionTarget {
readonly type: "ws";
@@ -37,14 +37,3 @@
): string | null {
return environment?.target.wsUrl ?? null;
}
-
-export function attachEnvironmentDescriptor(
- environment: KnownEnvironment,
- descriptor: ExecutionEnvironmentDescriptor,
-): KnownEnvironment {
- return {
- ...environment,
- environmentId: descriptor.environmentId,
- label: descriptor.label,
- };
-}
diff --git a/packages/client-runtime/src/scoped.ts b/packages/client-runtime/src/scoped.ts
--- a/packages/client-runtime/src/scoped.ts
+++ b/packages/client-runtime/src/scoped.ts
@@ -2,20 +2,10 @@
EnvironmentId,
ProjectId,
ScopedProjectRef,
- ScopedThreadSessionRef,
ScopedThreadRef,
ThreadId,
} from "@t3tools/contracts";
-interface EnvironmentScopedRef<TId extends string> {
- readonly environmentId: EnvironmentId;
- readonly id: TId;
-}
-
-export interface EnvironmentClientRegistry<TClient> {
- readonly getClient: (environmentId: EnvironmentId) => TClient | null | undefined;
-}
-
export function scopeProjectRef(
environmentId: EnvironmentId,
projectId: ProjectId,
@@ -27,34 +17,7 @@
return { environmentId, threadId };
}
-export function scopeThreadSessionRef(
- environmentId: EnvironmentId,
- threadId: ThreadId,
-): ScopedThreadSessionRef {
- return { environmentId, threadId };
-}
-
-export function scopedRefKey(
- ref: EnvironmentScopedRef<string> | ScopedProjectRef | ScopedThreadRef | ScopedThreadSessionRef,
-): string {
- const localId = "id" in ref ? ref.id : "projectId" in ref ? ref.projectId : ref.threadId;
+export function scopedRefKey(ref: ScopedProjectRef | ScopedThreadRef): string {
+ const localId = "projectId" in ref ? ref.projectId : ref.threadId;
return `${ref.environmentId}:${localId}`;
}
-
-export function resolveEnvironmentClient<TClient>(
- registry: EnvironmentClientRegistry<TClient>,
- ref: EnvironmentScopedRef<string>,
-): TClient {
- const client = registry.getClient(ref.environmentId);
- if (!client) {
- throw new Error(`No client registered for environment ${ref.environmentId}.`);
- }
- return client;
-}
-
-export function tagEnvironmentValue<T>(
- environmentId: EnvironmentId,
- value: T,
-): { readonly environmentId: EnvironmentId; readonly value: T } {
- return { environmentId, value };
-}You can send follow-ups to the cloud agent here.
juliusmarminge
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Member
Author
Contributor
|
Bugbot Autofix prepared a fix for the issue found in the latest run.
Or push these changes by commenting: Preview (b666cde275)diff --git a/apps/web/src/components/Sidebar.tsx b/apps/web/src/components/Sidebar.tsx
--- a/apps/web/src/components/Sidebar.tsx
+++ b/apps/web/src/components/Sidebar.tsx
@@ -58,7 +58,7 @@
import { APP_STAGE_LABEL, APP_VERSION } from "../branding";
import { isTerminalFocused } from "../lib/terminalFocus";
import { isLinuxPlatform, isMacPlatform, newCommandId, newProjectId } from "../lib/utils";
-import { getProjectScopedId, getThreadScopedId, useStore } from "../store";
+import { getProjectScopedId, useStore } from "../store";
import { selectThreadTerminalState, useTerminalStateStore } from "../terminalStateStore";
import { useUiStateStore } from "../uiStateStore";
import {
@@ -785,14 +785,8 @@
.filter((thread): thread is NonNullable<typeof thread> => thread !== undefined),
[sidebarThreadsByScopedId, threadScopedIdsByProjectScopedId],
);
- const sidebarThreadByScopedId = useMemo(
- () =>
- new Map(
- sidebarThreads.map((thread) => [
- getThreadScopedId({ environmentId: thread.environmentId, id: thread.id }),
- thread,
- ]),
- ),
+ const sidebarThreadById = useMemo(
+ () => new Map(sidebarThreads.map((thread) => [thread.id, thread] as const)),
[sidebarThreads],
);
const routeTerminalOpen = routeThreadId
@@ -1144,13 +1138,7 @@
if (clicked === "mark-unread") {
for (const id of ids) {
- let thread: SidebarThreadSummary | undefined;
- for (const t of sidebarThreadByScopedId.values()) {
- if (t.id === id) {
- thread = t;
- break;
- }
- }
+ const thread = sidebarThreadById.get(id);
markThreadUnread(id, thread?.latestTurn?.completedAt);
}
clearSelection();
@@ -1181,7 +1169,7 @@
deleteThread,
markThreadUnread,
removeFromSelection,
- sidebarThreadByScopedId,
+ sidebarThreadById,
selectedThreadIds,
],
);You can send follow-ups to the cloud agent here. |
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Sidebar
renderedThreadIdsdiverges fromrenderedThreadsusage- Removed the dead
renderedThreadIdscomputation from therenderedProjectsmemo and updatedgetVisibleSidebarThreadIdsto derive thread IDs fromrenderedThreadsinstead.
- Removed the dead
Or push these changes by commenting:
@cursor push b3888a4824
Preview (b3888a4824)
diff --git a/apps/web/src/components/Sidebar.logic.test.ts b/apps/web/src/components/Sidebar.logic.test.ts
--- a/apps/web/src/components/Sidebar.logic.test.ts
+++ b/apps/web/src/components/Sidebar.logic.test.ts
@@ -326,14 +326,17 @@
expect(
getVisibleSidebarThreadIds([
{
- renderedThreadIds: [
- ThreadId.makeUnsafe("thread-12"),
- ThreadId.makeUnsafe("thread-11"),
- ThreadId.makeUnsafe("thread-10"),
+ renderedThreads: [
+ { id: ThreadId.makeUnsafe("thread-12") },
+ { id: ThreadId.makeUnsafe("thread-11") },
+ { id: ThreadId.makeUnsafe("thread-10") },
],
},
{
- renderedThreadIds: [ThreadId.makeUnsafe("thread-8"), ThreadId.makeUnsafe("thread-6")],
+ renderedThreads: [
+ { id: ThreadId.makeUnsafe("thread-8") },
+ { id: ThreadId.makeUnsafe("thread-6") },
+ ],
},
]),
).toEqual([
@@ -350,14 +353,17 @@
getVisibleSidebarThreadIds([
{
shouldShowThreadPanel: false,
- renderedThreadIds: [
- ThreadId.makeUnsafe("thread-hidden-2"),
- ThreadId.makeUnsafe("thread-hidden-1"),
+ renderedThreads: [
+ { id: ThreadId.makeUnsafe("thread-hidden-2") },
+ { id: ThreadId.makeUnsafe("thread-hidden-1") },
],
},
{
shouldShowThreadPanel: true,
- renderedThreadIds: [ThreadId.makeUnsafe("thread-12"), ThreadId.makeUnsafe("thread-11")],
+ renderedThreads: [
+ { id: ThreadId.makeUnsafe("thread-12") },
+ { id: ThreadId.makeUnsafe("thread-11") },
+ ],
},
]),
).toEqual([ThreadId.makeUnsafe("thread-12"), ThreadId.makeUnsafe("thread-11")]);
diff --git a/apps/web/src/components/Sidebar.logic.ts b/apps/web/src/components/Sidebar.logic.ts
--- a/apps/web/src/components/Sidebar.logic.ts
+++ b/apps/web/src/components/Sidebar.logic.ts
@@ -230,14 +230,16 @@
return [...ordered, ...remaining];
}
-export function getVisibleSidebarThreadIds<TThreadId>(
+export function getVisibleSidebarThreadIds<TThread extends { id: unknown }>(
renderedProjects: readonly {
shouldShowThreadPanel?: boolean;
- renderedThreadIds: readonly TThreadId[];
+ renderedThreads: readonly TThread[];
}[],
-): TThreadId[] {
+): TThread["id"][] {
return renderedProjects.flatMap((renderedProject) =>
- renderedProject.shouldShowThreadPanel === false ? [] : renderedProject.renderedThreadIds,
+ renderedProject.shouldShowThreadPanel === false
+ ? []
+ : renderedProject.renderedThreads.map((thread) => thread.id),
);
}
diff --git a/apps/web/src/components/Sidebar.tsx b/apps/web/src/components/Sidebar.tsx
--- a/apps/web/src/components/Sidebar.tsx
+++ b/apps/web/src/components/Sidebar.tsx
@@ -1435,9 +1435,6 @@
const renderedThreads = pinnedCollapsedThread
? [pinnedCollapsedThread]
: visibleProjectThreads;
- const renderedThreadIds = pinnedCollapsedThread
- ? [pinnedCollapsedThread.id]
- : visibleProjectThreads.map((thread) => thread.id);
const showEmptyThreadState = project.expanded && projectThreads.length === 0;
return {
@@ -1447,7 +1444,6 @@
project,
projectStatus,
renderedThreads,
- renderedThreadIds,
showEmptyThreadState,
shouldShowThreadPanel,
isThreadListExpanded,You can send follow-ups to the cloud agent here.
- Persist a stable server environment ID and descriptor - Resolve repository identity from git remotes and enrich orchestration events - Thread environment metadata through desktop and web startup flows
- Add packages/client-runtime/package.json to the release smoke workspace list
- keep remote-host project/thread lookups separate from the active env - avoid double-enriching replayed project events
- Preserve GitLab subgroup paths when normalizing remotes - Re-resolve repository identity after remotes are added - Prefer the bootstrap URL when resolving the web socket server
- Make active environment mandatory in store and read-model sync - Scope chat, sidebar, and event replay state by resolved environment - Update tests for environment-aware thread and project handling
- keep untouched environment sidebar entries and project thread indexes stable during local snapshot sync - avoid eagerly resolving bootstrap URLs when an explicit server URL is provided - tighten scoped helpers by removing unused environment/session utilities
- add TTL-backed positive and negative caching for repository identity resolution - refresh identities when remotes appear or change after cache expiry - cover late-remote and remote-change cases in tests
- Strip explicit ports from URL-style git remotes - Add regression coverage for HTTPS and SSH remotes
- Preserve persisted environment IDs on read failures - Scope sidebar thread lookups to the active environment - Treat empty server URLs as unset
Co-authored-by: codex <codex@users.noreply.github.com>
juliusmarminge
force-pushed
the
t3code/remote-host-model
branch
from
314c455 to
54f905c
Compare
juliusmarminge
changed the base branch from
main
to
t3code/pr-1708/web/atomic-store-refactor
- Key web stores and routes by environment-aware thread refs - Update draft, terminal, and branch handling for scoped threads - Add tests for the new scoped selectors and routing
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Debug tool react-scan left in production HTML
- Removed the react-scan script tag from apps/web/index.html as it is a development-only profiling tool that should not be in production.
- ✅ Fixed: Draft thread errors silently swallowed by incorrect guard
- Changed the isCurrentServerThread guard to check
serverThread !== undefinedinstead ofactiveThread !== undefined, so draft thread errors correctly fall through to setLocalDraftErrorsByThreadId.
- Changed the isCurrentServerThread guard to check
Or push these changes by commenting:
@cursor push d45f9141fb
Preview (d45f9141fb)
diff --git a/apps/web/index.html b/apps/web/index.html
--- a/apps/web/index.html
+++ b/apps/web/index.html
@@ -12,7 +12,6 @@
rel="stylesheet"
/>
<title>T3 Code (Alpha)</title>
- <script crossorigin="anonymous" src="//unpkg.com/react-scan/dist/auto.global.js"></script>
</head>
<body>
<div id="root"></div>
diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx
--- a/apps/web/src/components/ChatView.tsx
+++ b/apps/web/src/components/ChatView.tsx
@@ -1695,10 +1695,10 @@
if (!targetThreadId) return;
const nextError = sanitizeThreadErrorMessage(error);
const isCurrentServerThread =
- activeThread !== undefined &&
+ serverThread !== undefined &&
targetThreadId === routeThreadRef.threadId &&
- activeThread.environmentId === routeThreadRef.environmentId &&
- activeThread.id === routeThreadRef.threadId;
+ serverThread.environmentId === routeThreadRef.environmentId &&
+ serverThread.id === routeThreadRef.threadId;
if (isCurrentServerThread) {
setStoreThreadError(targetThreadId, nextError);
return;
@@ -1713,7 +1713,7 @@
};
});
},
- [activeThread, routeThreadRef, setStoreThreadError],
+ [serverThread, routeThreadRef, setStoreThreadError],
);
const focusComposer = useCallback(() => {You can send follow-ups to the cloud agent here.
| rel="stylesheet" | ||
| /> | ||
| <title>T3 Code (Alpha)</title> | ||
| <script crossorigin="anonymous" src="//unpkg.com/react-scan/dist/auto.global.js"></script> |
Contributor
There was a problem hiding this comment.
Debug tool react-scan left in production HTML
High Severity
The react-scan profiling/debugging script is loaded from an external CDN (//unpkg.com/react-scan/dist/auto.global.js) in the production index.html. This development-only tool adds a visual overlay highlighting re-renders and will impact page load time, runtime performance, and user experience in production. It also introduces an external dependency on unpkg.com availability.
Reviewed by Cursor Bugbot for commit 010f452. Configure here.
| version: 2, | ||
| storage: createJSONStorage(createTerminalStateStorage), | ||
| migrate: (persistedState, version) => { | ||
| if (version === 2 && persistedState && typeof persistedState === "object") { |
Contributor
There was a problem hiding this comment.
🔴 Critical src/terminalStateStore.ts:826
The migrate function checks version === 2, but Zustand's persist middleware calls migrate when the stored version is older than the configured version. When a user has v1 data, migrate receives version=1, so the condition fails and the function returns { terminalStateByThreadKey: {} }, wiping all existing terminal state. The condition should be version === 1 or version < 2 to properly migrate v1 data.
- if (version === 2 && persistedState && typeof persistedState === "object") {
+ if (version === 1 && persistedState && typeof persistedState === "object") {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/terminalStateStore.ts around line 826:
The `migrate` function checks `version === 2`, but Zustand's persist middleware calls `migrate` when the stored version is *older* than the configured version. When a user has v1 data, `migrate` receives `version=1`, so the condition fails and the function returns `{ terminalStateByThreadKey: {} }`, wiping all existing terminal state. The condition should be `version === 1` or `version < 2` to properly migrate v1 data.
Evidence trail:
apps/web/src/terminalStateStore.ts lines 822-837 (REVIEWED_COMMIT): Shows `version: 2` and migrate function checking `version === 2` which fails for v1 data.
Zustand documentation at https://github.com/pmndrs/zustand/blob/main/docs/reference/integrations/persisting-store-data.md (Options > migrate section): States "The migrate function takes the persisted state and the version number as arguments" with example showing `if (version === 0)` to migrate from v0 to v1, confirming the version parameter is the stored/old version.
GitHub discussion https://github.com/pmndrs/zustand/discussions/1717: Confirms migrate receives `prevVersion` (the stored version) via example showing `migrate(prevState, prevVersion)`.
| rel="stylesheet" | ||
| /> | ||
| <title>T3 Code (Alpha)</title> | ||
| <script crossorigin="anonymous" src="//unpkg.com/react-scan/dist/auto.global.js"></script> |
Contributor
There was a problem hiding this comment.
🔴 Critical web/index.html:15
The react-scan debugging script is unconditionally loaded from //unpkg.com in index.html, so it executes in production builds without Subresource Integrity verification. If unpkg.com or the package is compromised, arbitrary JavaScript runs in users' browsers, and the performance overhead of component monitoring ships to all users.
- <script crossorigin="anonymous" src="//unpkg.com/react-scan/dist/auto.global.js"></script>🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/index.html around line 15:
The `react-scan` debugging script is unconditionally loaded from `//unpkg.com` in `index.html`, so it executes in production builds without Subresource Integrity verification. If unpkg.com or the package is compromised, arbitrary JavaScript runs in users' browsers, and the performance overhead of component monitoring ships to all users.
| window.cancelAnimationFrame(frame); | ||
| }; | ||
| }, [drawerHeight, resizeEpoch, terminalId, threadId]); | ||
| }, [drawerHeight, resizeEpoch, terminalId, threadId, threadRef]); |
Contributor
There was a problem hiding this comment.
🟡 Medium components/ThreadTerminalDrawer.tsx:720
The main useEffect in TerminalViewport uses threadRef inside the effect body (store subscription, openTerminal, selectTerminalEventEntries) but threadRef is omitted from the dependency array. If threadRef changes while threadId stays the same, the effect won't re-run and the closures will keep using the stale threadRef from the initial render. This causes selectTerminalEventEntries to query events with the wrong scoping key, so terminal events from the new scope are missed or events from the wrong scope are displayed.
Suggested change
| }, [drawerHeight, resizeEpoch, terminalId, threadId, threadRef]); | |
| }, [drawerHeight, resizeEpoch, terminalId, threadId, threadRef]); |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/components/ThreadTerminalDrawer.tsx around line 720:
The main `useEffect` in `TerminalViewport` uses `threadRef` inside the effect body (store subscription, `openTerminal`, `selectTerminalEventEntries`) but `threadRef` is omitted from the dependency array. If `threadRef` changes while `threadId` stays the same, the effect won't re-run and the closures will keep using the stale `threadRef` from the initial render. This causes `selectTerminalEventEntries` to query events with the wrong scoping key, so terminal events from the new scope are missed or events from the wrong scope are displayed.
Evidence trail:
apps/web/src/components/ThreadTerminalDrawer.tsx lines 258-681: useEffect with `[cwd, runtimeEnv, terminalId, threadId]` deps (line 681), `threadRef` used at lines 577-582, 608-611 but not in deps.
apps/web/src/terminalStateStore.ts lines 252, 538-543: `selectTerminalEventEntries` uses `terminalEventBufferKey(threadRef, terminalId)`.
packages/contracts/src/environment.ts lines 67-70: `ScopedThreadRef = { environmentId, threadId }`.
packages/client-runtime/src/scoped.ts lines 20-21: `scopedRefKey` produces `${ref.environmentId}:${localId}`.
| useComposerDraftStore.getState().clearComposerContent(threadId); | ||
|
|
||
| expect(useComposerDraftStore.getState().draftsByThreadId[threadId]).toBeUndefined(); | ||
| expect(draftFor(threadId, TEST_ENVIRONMENT_ID)).toBeUndefined(); |
Contributor
There was a problem hiding this comment.
🟡 Medium src/composerDraftStore.test.ts:318
The test assertion at line 318 checks draftFor(threadId, TEST_ENVIRONMENT_ID) but the draft was created using just threadId, which resolves to LEGACY_TEST_ENVIRONMENT_ID ("legacy"). Since TEST_ENVIRONMENT_ID ("environment-local") differs from the default, the assertion checks the wrong storage key and will always find undefined. Use draftFor(threadId) without the environment override to match the pattern at lines 307 and 349.
Suggested change
| expect(draftFor(threadId, TEST_ENVIRONMENT_ID)).toBeUndefined(); | |
| expect(draftFor(threadId)).toBeUndefined(); |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/composerDraftStore.test.ts around line 318:
The test assertion at line 318 checks `draftFor(threadId, TEST_ENVIRONMENT_ID)` but the draft was created using just `threadId`, which resolves to `LEGACY_TEST_ENVIRONMENT_ID` ("__legacy__"). Since `TEST_ENVIRONMENT_ID` ("environment-local") differs from the default, the assertion checks the wrong storage key and will always find undefined. Use `draftFor(threadId)` without the environment override to match the pattern at lines 307 and 349.
Evidence trail:
apps/web/src/composerDraftStore.test.ts lines 101-109 (TEST_ENVIRONMENT_ID='environment-local', LEGACY_TEST_ENVIRONMENT_ID='__legacy__', draftFor defaults to LEGACY), lines 311-318 (the test in question). apps/web/src/composerDraftStore.ts line 792 (LEGACY_COMPOSER_ENVIRONMENT_ID='__legacy__'), lines 803-831 (resolveComposerThreadRef falls back to LEGACY_COMPOSER_ENVIRONMENT_ID), lines 2250-2255 (addTerminalContext uses resolveComposerThreadRef).
Contributor
There was a problem hiding this comment.
🟠 High
t3code/apps/web/src/routes/__root.tsx
Line 275 in 010f452
Line 275 calls setProjectExpanded(payload.bootstrapProjectId, true) with a raw project ID, but syncProjects at lines 376-381 and 447-453 show that projectExpandedById now uses scoped keys generated by scopedProjectKey(scopeProjectRef(...)). The unscoped ID will not match any key in the map, so the project remains collapsed during bootstrap. Consider using the scoped key here, e.g. scopedProjectKey(scopeProjectRef(payload.environment.environmentId, payload.bootstrapProjectId)).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/routes/__root.tsx around line 275:
Line 275 calls `setProjectExpanded(payload.bootstrapProjectId, true)` with a raw project ID, but `syncProjects` at lines 376-381 and 447-453 show that `projectExpandedById` now uses scoped keys generated by `scopedProjectKey(scopeProjectRef(...))`. The unscoped ID will not match any key in the map, so the project remains collapsed during bootstrap. Consider using the scoped key here, e.g. `scopedProjectKey(scopeProjectRef(payload.environment.environmentId, payload.bootstrapProjectId))`.
Evidence trail:
apps/web/src/routes/__root.tsx:275 - calls `setProjectExpanded(payload.bootstrapProjectId, true)` with raw ID
apps/web/src/routes/__root.tsx:376-381 and 447-453 - `syncProjects` uses `scopedProjectKey(scopeProjectRef(...))`
apps/web/src/uiStateStore.ts:341-350 - `setProjectExpanded` uses projectId directly as key in `projectExpandedById`
apps/web/src/uiStateStore.ts:168-177 - `syncProjects` uses `project.key` (scoped key) for `nextExpandedById`
apps/web/src/components/Sidebar.tsx:773-777 - renders using `projectExpandedById[scopedProjectKey(scopeProjectRef(...))]`
packages/client-runtime/src/knownEnvironment.test.ts:42 - shows scoped key format is `"environment-test:project-1"`
Contributor
There was a problem hiding this comment.
🟠 High
t3code/apps/web/src/components/ChatView.tsx
Line 1693 in 010f452
The isCurrentServerThread check uses activeThread !== undefined, but activeThread can be a local draft thread (when serverThread is undefined and localDraftThread exists). When a draft thread's error is handled, isCurrentServerThread evaluates to true, so setStoreThreadError is called instead of updating localDraftErrorsByThreadId. Since draft threads don't exist in the app store, the error is silently dropped. Consider checking serverThread !== undefined to correctly distinguish server-backed threads from local drafts.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/components/ChatView.tsx around line 1693:
The `isCurrentServerThread` check uses `activeThread !== undefined`, but `activeThread` can be a local draft thread (when `serverThread` is undefined and `localDraftThread` exists). When a draft thread's error is handled, `isCurrentServerThread` evaluates to `true`, so `setStoreThreadError` is called instead of updating `localDraftErrorsByThreadId`. Since draft threads don't exist in the app store, the error is silently dropped. Consider checking `serverThread !== undefined` to correctly distinguish server-backed threads from local drafts.
Evidence trail:
apps/web/src/components/ChatView.tsx line 907: `const activeThread = serverThread ?? localDraftThread;` shows activeThread can be a draft thread
apps/web/src/components/ChatView.tsx line 913: `const isServerThread = serverThread !== undefined;` shows the correct pattern exists in codebase
apps/web/src/components/ChatView.tsx lines 1700-1704: `isCurrentServerThread` uses `activeThread !== undefined` instead of `serverThread !== undefined`
apps/web/src/store.ts lines 917-930: `updateThreadState` returns unchanged state when thread doesn't exist in store
apps/web/src/store.ts lines 1679-1693: `setError` relies on `updateThreadState` which will no-op for non-existent threads
- Route composer drafts through `/draft/:threadId` - Persist logical project draft mappings and update store lookups - Adjust chat view, navigation, and tests for the new draft flow
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Tests use raw threadId instead of scoped key
- Replaced all five
draftsByThreadKey[newThreadId]/draftsByThreadKey[threadId]lookups withdraftsByThreadKey[threadKeyFor(...)]to use the correct scoped key format expected by the store.
- Replaced all five
Or push these changes by commenting:
@cursor push 690b18e97e
Preview (690b18e97e)
diff --git a/apps/web/src/components/ChatView.browser.tsx b/apps/web/src/components/ChatView.browser.tsx
--- a/apps/web/src/components/ChatView.browser.tsx
+++ b/apps/web/src/components/ChatView.browser.tsx
@@ -2560,7 +2560,9 @@
);
const newThreadId = threadIdFromPath(newThreadPath);
- expect(useComposerDraftStore.getState().draftsByThreadKey[newThreadId]).toMatchObject({
+ expect(
+ useComposerDraftStore.getState().draftsByThreadKey[threadKeyFor(newThreadId)],
+ ).toMatchObject({
modelSelectionByProvider: {
codex: {
provider: "codex",
@@ -2613,7 +2615,9 @@
);
const newThreadId = threadIdFromPath(newThreadPath);
- expect(useComposerDraftStore.getState().draftsByThreadKey[newThreadId]).toMatchObject({
+ expect(
+ useComposerDraftStore.getState().draftsByThreadKey[threadKeyFor(newThreadId)],
+ ).toMatchObject({
modelSelectionByProvider: {
claudeAgent: {
provider: "claudeAgent",
@@ -2653,7 +2657,9 @@
);
const newThreadId = threadIdFromPath(newThreadPath);
- expect(useComposerDraftStore.getState().draftsByThreadKey[newThreadId]).toBe(undefined);
+ expect(useComposerDraftStore.getState().draftsByThreadKey[threadKeyFor(newThreadId)]).toBe(
+ undefined,
+ );
} finally {
await mounted.cleanup();
}
@@ -2695,7 +2701,9 @@
);
const threadId = threadIdFromPath(threadPath);
- expect(useComposerDraftStore.getState().draftsByThreadKey[threadId]).toMatchObject({
+ expect(
+ useComposerDraftStore.getState().draftsByThreadKey[threadKeyFor(threadId)],
+ ).toMatchObject({
modelSelectionByProvider: {
codex: {
provider: "codex",
@@ -2724,7 +2732,9 @@
(path) => path === threadPath,
"New-thread should reuse the existing project draft thread.",
);
- expect(useComposerDraftStore.getState().draftsByThreadKey[threadId]).toMatchObject({
+ expect(
+ useComposerDraftStore.getState().draftsByThreadKey[threadKeyFor(threadId)],
+ ).toMatchObject({
modelSelectionByProvider: {
codex: {
provider: "codex",You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 24ac444. Configure here.
| const newThreadId = threadIdFromPath(newThreadPath); | ||
|
|
||
| expect(useComposerDraftStore.getState().draftsByThreadId[newThreadId]).toMatchObject({ | ||
| expect(useComposerDraftStore.getState().draftsByThreadKey[newThreadId]).toMatchObject({ |
Contributor
There was a problem hiding this comment.
Tests use raw threadId instead of scoped key
Medium Severity
threadIdFromPath returns a raw ThreadId (e.g. "abc-123"), but this value is then used to index into draftsByThreadKey, which is keyed by scoped thread keys in the format "environmentId:threadId" (e.g. "environment-local:abc-123"). This means toMatchObject assertions will fail or toBe(undefined) checks will pass vacuously, making the tests ineffective at validating draft composer state after thread creation.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 24ac444. Configure here.
Contributor
There was a problem hiding this comment.
🟡 Medium
t3code/apps/web/src/components/ChatView.tsx
Line 3070 in 24ac444
When a draft worktree thread is missing a branch, setStoreThreadError is called directly at line 3070 instead of setThreadError. Since draft threads don't exist in the server store's environment state, updateThreadState inside setError is a no-op and the error message "Select a base branch before sending in New worktree mode." is silently dropped. Use setThreadError(threadIdForSend, ...) instead.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/components/ChatView.tsx around line 3070:
When a draft worktree thread is missing a branch, `setStoreThreadError` is called directly at line 3070 instead of `setThreadError`. Since draft threads don't exist in the server store's environment state, `updateThreadState` inside `setError` is a no-op and the error message `"Select a base branch before sending in New worktree mode."` is silently dropped. Use `setThreadError(threadIdForSend, ...)` instead.
Evidence trail:
apps/web/src/components/ChatView.tsx lines 3069-3074 (setStoreThreadError call), lines 1719-1742 (setThreadError implementation showing draft thread handling), lines 918-919 (isServerThread and activeThread definitions), apps/web/src/store.ts lines 918-932 (updateThreadState returning unchanged state when thread doesn't exist), lines 1684-1698 (setError calling updateThreadState)
Comment on lines
+2585
to
+2595
| const draftThreadEntries = Object.entries(store.draftThreadsByThreadKey).flatMap( | ||
| ([threadKey]) => { | ||
| const threadRef = composerThreadRefFromKey(threadKey); | ||
| return threadRef?.threadId === threadId ? [threadRef] : []; | ||
| }, | ||
| ); | ||
| if (draftThreadEntries.length !== 1) { | ||
| return; | ||
| } | ||
| clearPromotedDraftThreadByRef(draftThreadEntries[0]!); | ||
| } |
Contributor
There was a problem hiding this comment.
🟢 Low src/composerDraftStore.ts:2585
When draftThreadEntries.length > 1 (multiple draft entries reference the same threadId under different keys), the function silently returns without clearing any of them. Since the function's purpose is to "clear a draft thread once the server has materialized the same thread id," having multiple draft entries for a promoted server thread should still result in cleanup, not silent abandonment. This could leave orphaned draft data.
+ const draftThreadEntries = Object.entries(store.draftThreadsByThreadKey).flatMap(
+ ([threadKey]) => {
+ const threadRef = composerThreadRefFromKey(threadKey);
+ return threadRef?.threadId === threadId ? [threadRef] : [];
+ },
+ );
+ for (const draftThreadRef of draftThreadEntries) {
+ clearPromotedDraftThreadByRef(draftThreadRef);
+ }🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/composerDraftStore.ts around lines 2585-2595:
When `draftThreadEntries.length > 1` (multiple draft entries reference the same `threadId` under different keys), the function silently returns without clearing any of them. Since the function's purpose is to "clear a draft thread once the server has materialized the same thread id," having multiple draft entries for a promoted server thread should still result in cleanup, not silent abandonment. This could leave orphaned draft data.
Evidence trail:
apps/web/src/composerDraftStore.ts lines 2579-2596 (REVIEWED_COMMIT): The `clearPromotedDraftThread` function uses `if (draftThreadEntries.length !== 1) { return; }` which causes silent return without cleanup when multiple draft entries exist for the same threadId. The docstring at lines 2570-2576 states the purpose is to "Clear a draft thread once the server has materialized the same thread id."
- Route UI actions through environment-specific native APIs - Include environment IDs in git/react-query keys and callers - Update affected tests and timeline components
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
prep work for multi-environments
Summary
Testing
bun fmt- Not run.bun lint- Not run.bun typecheck- Not run.bun run test- Not run.Note
Medium Risk
Touches server startup and websocket payloads by introducing environment descriptors and enriching project events/snapshots with repository identity derived from
gitremotes; this can affect client compatibility and performance if resolution is slow or errors. Desktop adds a new sync IPC channel, and web UI updates require the new environment-scoped APIs/state, increasing integration risk.Overview
Adds a first-class execution environment concept: the server now persists a stable
environmentIdon disk (environment-id) and exposes anExecutionEnvironmentDescriptor(label, platform, server version, capability flags) via newServerEnvironmentservice, which is included inwelcome/readylifecycle payloads andServerConfigresponses.Introduces
RepositoryIdentityResolver(cachedgit remote -vnormalization) and wires it through orchestration: project snapshots now includerepositoryIdentity, and websocketreplayEventsplus live orchestration domain events are enriched with repository identity forproject.*events.Desktop adds a new sync IPC channel
desktop:get-local-environment-bootstrapexposed viadesktopBridge.getLocalEnvironmentBootstrap()to let the renderer fetch the local environment label and WS URL. Tests are expanded/updated for environment ID persistence, repository identity resolution/caching, projection snapshots, and websocket event enrichment; a few long-running git tests get higher timeouts, and web adds@t3tools/client-runtimeplus areact-scanscript include for instrumentation.Reviewed by Cursor Bugbot for commit 46c3251. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Scope all thread, project, and git state by environment to prepare for multi-environment support
EnvironmentId,ScopedThreadRef, andScopedProjectRefcontracts and a new@t3tools/client-runtimepackage with helpers to create and parse scoped keys.AppStateintoenvironmentStateById: Record<string, EnvironmentState>with anactiveEnvironmentIdpointer; all selectors, mutations, and orchestration event handlers are now environment-scoped.draftsByThreadId→draftsByThreadKey,terminalStateByThreadId→terminalStateByThreadKey,selectedThreadIds→selectedThreadKeys, and similar across composer draft, terminal, thread selection, and UI state stores.ServerEnvironmentEffect service that persists a stable environment ID to disk and exposes anExecutionEnvironmentDescriptor; startup/welcome payloads and RPC responses now include this descriptor.RepositoryIdentityResolverservice that caches git remote lookups (with positive/negative TTLs) and enrichesproject.created/project.meta-updatedorchestration events withrepositoryIdentity.KnownEnvironment, addingensureWsRpcClientEntryForKnownEnvironment,getWsRpcClientForEnvironment, andbindPrimaryWsRpcClientEnvironment.gitReactQuery,gitStatusState,projectReactQuery,providerReactQuery) to require and includeenvironmentIdin query/mutation keys and API calls./_chat/$threadIdto/_chat/$environmentId/$threadId; a new/_chat/draft/$threadIdroute handles local draft threads.Macroscope summarized 46c3251.